Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Great job on task-list-api and nice job making frequent commits!
Let me know if you have questions about my comments.
| from .routes.task_routes import bp as tasks_bp | ||
| from .routes.goal_routes import bp as goals_bp |
|
|
||
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| new_goal= cls(title=goal_data["title"]) |
There was a problem hiding this comment.
Nitpick: spacing
| new_goal= cls(title=goal_data["title"]) | |
| new_goal = cls(title=goal_data["title"]) |
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| description: Mapped[str] | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) |
There was a problem hiding this comment.
Using Optional[] is all that we need to indicate that a field is nullable so we don't need to also include mapped_column with nullable=True.
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) | |
| completed_at: Mapped[Optional[datetime]] |
Here's the SQLAlchemy documentation about nullability
| return dict( | ||
| id = goal.id, | ||
| title = goal.title, | ||
| tasks = [task.to_dict() for task in goal.tasks] | ||
| ) |
There was a problem hiding this comment.
Could we use the to_dict method you wrote in Goal?
| return dict( | |
| id = goal.id, | |
| title = goal.title, | |
| tasks = [task.to_dict() for task in goal.tasks] | |
| ) | |
| return goal.to_dict() |
| for task_in_goal in goal.tasks: | ||
| if task_in_goal.id not in task_ids: | ||
| task_in_goal.goal_id = None | ||
|
|
||
| valid_tasks_ids = [] | ||
|
|
||
| # Assign valid tasks to this goal | ||
| for task in valid_tasks: | ||
| valid_tasks_ids.append(task.id) | ||
| if task.goal_id != goal.id: | ||
| task.goal_id = goal.id |
There was a problem hiding this comment.
What if we don't need to unlink the tasks because we can just replace them?
| for task_in_goal in goal.tasks: | |
| if task_in_goal.id not in task_ids: | |
| task_in_goal.goal_id = None | |
| valid_tasks_ids = [] | |
| # Assign valid tasks to this goal | |
| for task in valid_tasks: | |
| valid_tasks_ids.append(task.id) | |
| if task.goal_id != goal.id: | |
| task.goal_id = goal.id | |
| tasks = [validate_model(task_id) for task_id in task_ids] | |
| goal.tasks = tasks |
| try: | ||
| new_model = cls.from_dict(model_data) | ||
| except KeyError as error: | ||
| response = {"details": "Invalid data"} |
There was a problem hiding this comment.
It might be nice to provide even more details in the response you send back so that the client knows how to fix up the request body they will re-send.
| response = {"details": "Invalid data"} | |
| response = {"details": f"Invalid data for field: {error.args[0]}"} |
For example, this would evaluate to "title" for example if I sent a bad request body to create a Task.
| path = "https://slack.com/api/chat.postMessage" | ||
| API_KEY = os.environ.get("API_KEY") | ||
| headers = {"Authorization": f"Bearer {API_KEY}"} | ||
| body ={ | ||
| "channel": "task-notifications", | ||
| "text": f"Someone just completed the task {task.title}" | ||
| } | ||
|
|
||
| if not task.completed_at: | ||
| task.completed_at = datetime.now() | ||
| slack_post = requests.post(path, headers=headers, json=body ) |
There was a problem hiding this comment.
Prefer all the logic related to calling Slack to be in a helper function (maybe like call_slack_api) to make this route more concise and single-responsibility.
| """ | ||
| task = validate_model(Task, task_id) | ||
|
|
||
| path = "https://slack.com/api/chat.postMessage" |
There was a problem hiding this comment.
This string is a constant value so we should name the variable with all capital letters.
| path = "https://slack.com/api/chat.postMessage" | |
| PATH = "https://slack.com/api/chat.postMessage" |
| body ={ | ||
| "channel": "task-notifications", |
There was a problem hiding this comment.
Nitpick: spacing
Also prefer the channel to be referenced by a constant variable too.
| body ={ | |
| "channel": "task-notifications", | |
| SLACK_CHANNEL = "task-notifications" | |
| body = { | |
| "channel": SLACK_CHANNEL, |
|
|
||
| if not task.completed_at: | ||
| task.completed_at = datetime.now() | ||
| slack_post = requests.post(path, headers=headers, json=body ) |
There was a problem hiding this comment.
Nitpick: spacing
| slack_post = requests.post(path, headers=headers, json=body ) | |
| slack_post = requests.post(path, headers=headers, json=body) |
2 participants
No description provided.